Fix program lock waiting issue #3978
Conversation
|
🥷 Code experts: VictoriousRaptor, jjw24 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
|
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes potential program lock issues by implementing proper lock release patterns using try-finally blocks throughout the codebase.
- Wraps lock-protected code sections in try-finally blocks to ensure semaphore locks are always released
- Refactors program disabling logic to fix lock release patterns and prevent deadlocks
- Makes minor optimizations including collection initialization and comparison improvements
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ProgramSettingDisplay.cs | Adds try-finally blocks around semaphore-protected operations to ensure locks are always released |
| Main.cs | Implements proper lock handling patterns and refactors program disabling logic to prevent lock issues |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughRefactors locking and cache-loading in the Program plugin to use try/finally for guaranteed lock release, adjusts empty-collection checks, and defers reindex triggers until after locks are released. Applies across initialization, file-move handling, disabling programs, and UI command views. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI (Settings/Actions)
participant Plugin as Program Plugin
participant WLock as _win32sLock
participant ULock as _uwpsLock
participant Indexer as Reindex Tasks
rect rgb(240,248,255)
note over Plugin: DisableProgramAsync (modified flow)
UI->>Plugin: Disable program
alt Win32 target
Plugin->>WLock: Acquire()
Note over Plugin,WLock: try
Plugin-->>Plugin: Update win32 cache/state
Plugin-->>Plugin: set reindexWin32s flag
Plugin->>WLock: Release() (finally)
opt reindex needed
Plugin->>Indexer: Task.Run(IndexWin32ProgramsAsync)
end
else UWP target
Plugin->>ULock: Acquire()
Note over Plugin,ULock: try
Plugin-->>Plugin: Update UWP cache/state
Plugin-->>Plugin: set reindexUwps flag
Plugin->>ULock: Release() (finally)
opt reindex needed
Plugin->>Indexer: Task.Run(IndexUwpProgramsAsync)
end
end
end
rect rgb(245,255,245)
note over Plugin: InitAsync / MoveFile preload & reload
Plugin->>WLock: Acquire()
Note over Plugin,WLock: try
Plugin-->>Plugin: Load Win32 cache
Plugin->>WLock: Release() (finally)
Plugin->>ULock: Acquire()
Note over Plugin,ULock: try
Plugin-->>Plugin: Load UWP cache
Plugin->>ULock: Release() (finally)
end
rect rgb(255,250,240)
note over UI,Plugin: DisplayAllProgramsAsync / SetProgramSourcesStatusAsync
UI->>Plugin: Display / Toggle sources
Plugin->>WLock: Acquire()
Note over Plugin,WLock: try
Plugin-->>UI: Enumerate/update Win32 items
Plugin->>WLock: Release() (finally)
Plugin->>ULock: Acquire()
Note over Plugin,ULock: try
Plugin-->>UI: Enumerate/update UWP items
Plugin->>ULock: Release() (finally)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Plugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs (2)
114-116: Reindex condition uses AND; should be OR.If an enabled item is missing from either cache, we should reindex; the current AND can skip required reindex.
- if (selectedItems.Any(t1 => t1.Enabled && !Main._uwps.Any(x => t1.UniqueIdentifier == x.UniqueIdentifier)) - && selectedItems.Any(t1 => t1.Enabled && !Main._win32s.Any(x => t1.UniqueIdentifier == x.UniqueIdentifier))) + if (selectedItems.Any(t1 => t1.Enabled && !Main._uwps.Any(x => t1.UniqueIdentifier == x.UniqueIdentifier)) + || selectedItems.Any(t1 => t1.Enabled && !Main._win32s.Any(x => t1.UniqueIdentifier == x.UniqueIdentifier))) return true;
102-105: Remove by identity, not by stale Enabled flag.ProgramSource instances in DisabledProgramSources don’t get their Enabled toggled in this flow, so RemoveAll(t1 => t1.Enabled) won’t purge re‑enabled items.
-internal static void RemoveDisabledFromSettings() -{ - Main._settings.DisabledProgramSources.RemoveAll(t1 => t1.Enabled); -} +internal static void RemoveDisabledFromSettings() +{ + // Remove any disabled entries that now appear enabled in the display list + var enabledIds = new HashSet<string>( + ProgramSetting.ProgramSettingDisplayList.Where(ps => ps.Enabled).Select(ps => ps.UniqueIdentifier)); + Main._settings.DisabledProgramSources.RemoveAll(d => enabledIds.Contains(d.UniqueIdentifier)); +}
🧹 Nitpick comments (4)
Plugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs (1)
50-56: Avoid repeated Any() scans; precompute a lookup.Current O(n*m) checks per collection. Use a HashSet of UniqueIdentifier for O(1) membership.
-foreach(var program in ProgramSetting.ProgramSettingDisplayList) +var targetIds = new HashSet<string>(selectedProgramSourcesToDisable.Select(s => s.UniqueIdentifier)); +foreach (var program in ProgramSetting.ProgramSettingDisplayList) { - if (selectedProgramSourcesToDisable.Any(x => x.UniqueIdentifier == program.UniqueIdentifier && program.Enabled != status)) + if (targetIds.Contains(program.UniqueIdentifier) && program.Enabled != status) { program.Enabled = status; } } @@ -foreach (var program in Main._win32s) +foreach (var program in Main._win32s) { - if (selectedProgramSourcesToDisable.Any(x => x.UniqueIdentifier == program.UniqueIdentifier && program.Enabled != status)) + if (targetIds.Contains(program.UniqueIdentifier) && program.Enabled != status) { program.Enabled = status; } } @@ -foreach (var program in Main._uwps) +foreach (var program in Main._uwps) { - if (selectedProgramSourcesToDisable.Any(x => x.UniqueIdentifier == program.UniqueIdentifier && program.Enabled != status)) + if (targetIds.Contains(program.UniqueIdentifier) && program.Enabled != status) { program.Enabled = status; } }Also applies to: 61-67, 77-83
Plugins/Flow.Launcher.Plugin.Program/Main.cs (3)
85-116: Shorten lock hold time by snapshotting collections.Query holds both locks across PLINQ + p.Result(), blocking writers. Snapshot under lock, then process outside.
- await _win32sLock.WaitAsync(token); - await _uwpsLock.WaitAsync(token); - try - { - // Collect all UWP Windows app directories - var uwpsDirectories = _settings.HideDuplicatedWindowsApp ? _uwps + List<IProgram> progsSnapshot; + string[]? uwpsDirectories; + await _win32sLock.WaitAsync(token); + await _uwpsLock.WaitAsync(token); + try + { + var uwpsLocal = _uwps.ToArray(); + var win32Local = _win32s.Cast<IProgram>().ToArray(); + progsSnapshot = win32Local.Concat(uwpsLocal).ToList(); + uwpsDirectories = _settings.HideDuplicatedWindowsApp ? uwpsLocal .Where(uwp => !string.IsNullOrEmpty(uwp.Location)) // Exclude invalid paths .Where(uwp => uwp.Location.StartsWith(WindowsAppPath, StringComparison.OrdinalIgnoreCase)) // Keep system apps .Select(uwp => uwp.Location.TrimEnd('\\')) // Remove trailing slash .Distinct(StringComparer.OrdinalIgnoreCase) .ToArray() : null; - - return _win32s.Cast<IProgram>() - .Concat(_uwps) + } + finally + { + _uwpsLock.Release(); + _win32sLock.Release(); + } + + try + { + return progsSnapshot .AsParallel() .WithCancellation(token) .Where(HideUninstallersFilter) .Where(p => HideDuplicatedWindowsAppFilter(p, uwpsDirectories)) .Where(p => p.Enabled) .Select(p => p.Result(query.Search, Context.API)) .Where(r => string.IsNullOrEmpty(query.Search) || r?.Score > 0) .ToList(); } - catch (OperationCanceledException) + catch (OperationCanceledException) { return emptyResults; - } - finally - { - _uwpsLock.Release(); - _win32sLock.Release(); - } + }
168-171: Null-guard before TrimEnd to avoid NRE.- var location = program.Location.TrimEnd('\\'); // Ensure trailing slash - if (string.IsNullOrEmpty(location)) + var loc = program.Location; + if (string.IsNullOrEmpty(loc)) return true; // Keep if location is invalid + var location = loc.TrimEnd('\\'); // Ensure trailing slash
363-368: Cache reset can race with in-flight GetOrCreateAsync calls.Disposing the old cache while other threads may still reference it can throw. Prefer cache.Compact(1.0) or guard with a lock + publish the new cache without disposing the old one immediately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Plugins/Flow.Launcher.Plugin.Program/Main.cs(4 hunks)Plugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.csPlugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.csPlugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs
📚 Learning: 2025-07-06T12:21:37.947Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3572
File: Flow.Launcher/App.xaml.cs:214-216
Timestamp: 2025-07-06T12:21:37.947Z
Learning: In Flow Launcher, the UpdatePluginManifestAsync method in PluginsManifest.cs already has comprehensive internal try-catch handling that logs exceptions and returns false on failure rather than throwing, making external try-catch wrappers unnecessary.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.csPlugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs
🧬 Code graph analysis (2)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (4)
Plugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs (4)
List(10-17)Task(19-46)Task(48-89)Task(107-131)Flow.Launcher/PublicAPIInstance.cs (10)
List(252-252)List(519-519)Task(120-120)Task(225-244)Task(257-258)Task(260-261)Task(263-264)Task(516-517)Task(549-556)Task(558-565)Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (4)
List(475-509)UWPApp(199-238)UWPApp(387-395)Task(295-323)Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs (9)
List(217-276)List(832-853)Win32(300-333)Win32(335-391)Win32(393-431)Win32(433-457)Win32(595-610)Win32(671-726)Task(789-801)
Plugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs (2)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
Main(18-489)Plugins/Flow.Launcher.Plugin.Program/Views/ProgramSetting.xaml.cs (2)
ProgramSetting(19-465)ProgramSetting(112-118)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: gitStream.cm
- GitHub Check: Report (PR)
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (3)
Plugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs (2)
21-32: Good: lock release guaranteed for Win32 block.The try/finally around _win32sLock is correct and resolves the prior potential leak.
34-45: Good: lock release guaranteed for UWP block.Mirrors the Win32 pattern; safe and consistent.
Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
34-34: Projects target .NET 9.0 (C# 12+): no LangVersion change required
All .csproj files target net9.0-windows*, whose SDK default LangVersion is ≥ C# 12, so the collection expression[]is supported without an explicit<LangVersion>setting. Ensure your local/CI build uses .NET SDK ≥ 8.0 (ideally 9.0+) to pick up C# 12 features.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
85-116: Fix lock leak on cancellation when acquiring both locks.If the second WaitAsync(token) throws before entering the try block, _win32sLock stays held, causing a deadlock for subsequent queries. Acquire inside try and release conditionally.
Apply this diff:
- await _win32sLock.WaitAsync(token); - await _uwpsLock.WaitAsync(token); - try + var gotWin32 = false; + var gotUwp = false; + try { + await _win32sLock.WaitAsync(token); + gotWin32 = true; + await _uwpsLock.WaitAsync(token); + gotUwp = true; // Collect all UWP Windows app directories var uwpsDirectories = _settings.HideDuplicatedWindowsApp ? _uwps .Where(uwp => !string.IsNullOrEmpty(uwp.Location)) // Exclude invalid paths .Where(uwp => uwp.Location.StartsWith(WindowsAppPath, StringComparison.OrdinalIgnoreCase)) // Keep system apps .Select(uwp => uwp.Location.TrimEnd('\\')) // Remove trailing slash .Distinct(StringComparer.OrdinalIgnoreCase) .ToArray() : null; @@ - finally - { - _uwpsLock.Release(); - _win32sLock.Release(); - } + finally + { + if (gotUwp) _uwpsLock.Release(); + if (gotWin32) _win32sLock.Release(); + }
♻️ Duplicate comments (2)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (2)
421-433: First() can throw; compute presence via FirstOrDefault and set reindex flag accordingly. Also initialize flag to false.Current code sets the flag via Any(...) but still calls First(...), which will throw if not found.
Apply this diff:
- await _uwpsLock.WaitAsync(); - var reindexUwps = true; + await _uwpsLock.WaitAsync(); + var reindexUwps = false; try { - reindexUwps = _uwps.Any(x => x.UniqueIdentifier == programToDelete.UniqueIdentifier); - var program = _uwps.First(x => x.UniqueIdentifier == programToDelete.UniqueIdentifier); - program.Enabled = false; - _settings.DisabledProgramSources.Add(new ProgramSource(program)); + var uwp = _uwps.FirstOrDefault(x => x.UniqueIdentifier == programToDelete.UniqueIdentifier); + if (uwp != null) + { + uwp.Enabled = false; + _settings.DisabledProgramSources.Add(new ProgramSource(uwp)); + reindexUwps = true; + } } finally { _uwpsLock.Release(); }(Optional: rename reindexUwps to foundInUwps for clarity.)
443-455: Same issue for Win32 block: avoid First() on missing item and fix flag semantics.Mirror the UWP fix to prevent exceptions and misleading initialization.
- await _win32sLock.WaitAsync(); - var reindexWin32s = true; + await _win32sLock.WaitAsync(); + var reindexWin32s = false; try { - reindexWin32s = _win32s.Any(x => x.UniqueIdentifier == programToDelete.UniqueIdentifier); - var program = _win32s.First(x => x.UniqueIdentifier == programToDelete.UniqueIdentifier); - program.Enabled = false; - _settings.DisabledProgramSources.Add(new ProgramSource(program)); + var win = _win32s.FirstOrDefault(x => x.UniqueIdentifier == programToDelete.UniqueIdentifier); + if (win != null) + { + win.Enabled = false; + _settings.DisabledProgramSources.Add(new ProgramSource(win)); + reindexWin32s = true; + } } finally { _win32sLock.Release(); }
🧹 Nitpick comments (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (1)
119-119: Micro‑nit: simpler empty assignment.Equivalent but clearer and avoids a ternary.
- resultList = resultList.Count != 0 ? resultList : emptyResults; + if (resultList.Count == 0) resultList = emptyResults;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-21T09:19:49.684Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher/App.xaml.cs:246-262
Timestamp: 2025-07-21T09:19:49.684Z
Learning: In Flow Launcher's App.xaml.cs, the asynchronous plugin initialization task (containing AbstractPluginEnvironment.PreStartPluginExecutablePathUpdate, PluginManager.LoadPlugins, PluginManager.InitializePluginsAsync, and AutoPluginUpdates) does not require additional try-catch error handling according to maintainer Jack251970, as these operations are designed to handle exceptions internally.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
📚 Learning: 2025-07-21T09:19:19.012Z
Learnt from: Jack251970
PR: Flow-Launcher/Flow.Launcher#3854
File: Flow.Launcher.Core/Plugin/PluginManager.cs:280-292
Timestamp: 2025-07-21T09:19:19.012Z
Learning: In Flow Launcher's PluginManager.cs, the post-initialization operations (RegisterResultsUpdatedEvent, UpdatePluginMetadataTranslation, RegisterPluginActionKeywords, DialogJump.InitializeDialogJumpPlugin, and AddPluginToLists) are designed to be exception-safe and do not require additional try-catch error handling according to the maintainer Jack251970.
Applied to files:
Plugins/Flow.Launcher.Plugin.Program/Main.cs
🧬 Code graph analysis (1)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (4)
Plugins/Flow.Launcher.Plugin.Program/Views/Commands/ProgramSettingDisplay.cs (4)
List(10-17)Task(19-46)Task(48-89)Task(107-131)Flow.Launcher/PublicAPIInstance.cs (10)
List(252-252)List(519-519)Task(120-120)Task(225-244)Task(257-258)Task(260-261)Task(263-264)Task(516-517)Task(549-556)Task(558-565)Plugins/Flow.Launcher.Plugin.Program/Programs/UWPPackage.cs (4)
List(475-509)UWPApp(199-238)UWPApp(387-395)Task(295-323)Plugins/Flow.Launcher.Plugin.Program/Programs/Win32.cs (9)
List(217-276)List(832-853)Win32(300-333)Win32(335-391)Win32(393-431)Win32(433-457)Win32(595-610)Win32(671-726)Task(789-801)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: gitStream workflow automation
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: gitStream.cm
- GitHub Check: build
🔇 Additional comments (5)
Plugins/Flow.Launcher.Plugin.Program/Main.cs (5)
251-260: Good: lock release guaranteed on cache load (Win32).Wrapping the cache load in try/finally removes the risk of over/under‑release on exceptions.
263-271: Good: lock release guaranteed on cache load (UWP).Same improvement here; this addresses the “program lock issue” during preload.
435-441: Confirm early‑return behavior.If the program exists in both caches (edge case), this early return means only UWP is disabled/reindexed. Is that intentional?
456-461: LGTM once flag logic above is corrected.Reindex gating and deferring the background task until after release is the right pattern.
Please re-run a manual test: disable a Win32 program, confirm it disappears after reindex; disable a UWP app, confirm likewise.
34-34: No change required — net9 target uses C# 13 so collection expression [] is supported.The projects target net9.0 and .NET 9 defaults to C# 13; C# 13 includes C# 12’s collection expressions (the [] literal), so the line
private static readonly List<Result> emptyResults = [];is valid as-is. (learn.microsoft.com)
| internal static PluginInitContext Context { get; private set; } | ||
|
|
||
| private static readonly List<Result> emptyResults = new(); | ||
| private static readonly List<Result> emptyResults = []; |
Fix program lock waiting issue
2 participants
Use
tryfinallysentence to fix program lock waiting issue